-
Notifications
You must be signed in to change notification settings - Fork 107
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
use pytest rather than python -m pytest #747
Merged
Vincent-Maladiere
merged 8 commits into
skrub-data:main
from
jeromedockes:fix_pytest_command
Sep 29, 2023
Merged
use pytest rather than python -m pytest #747
Vincent-Maladiere
merged 8 commits into
skrub-data:main
from
jeromedockes:fix_pytest_command
Sep 29, 2023
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jeromedockes
force-pushed
the
fix_pytest_command
branch
from
September 21, 2023 12:29
3a93364
to
2f2e1d1
Compare
the failure is not related to this but to #748 |
Vincent-Maladiere
approved these changes
Sep 27, 2023
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we also need to discuss adding a "python" or "src" folder to avoid importing the local files rather than using the installed package?
Do we also need to discuss adding a "python" or "src" folder to avoid importing the local files rather than using the installed package?
this PR already prevents importing the local files when testing in the
CI. adding a src folder would be an extra safety net to catch this kind
of mistake in case we mess up the CI in the future, so it is not
strictly necessary if the CI remains configured correctly. I still think
it could be marginally useful because a misconfigured CI that passes
when it should fail can be hard to detect before users report defects
after a release. However, Gaël was not a fan because it would make the
layout of skrub more different from scikit-learn. I don't have a very
strong opinion, happy to discuss it some more.
Locally during development, the package is likely to be installed in
editable mode so the files in the repo will be imported during tests
anyway, unless we use something like tox. It could be useful to add a
tox config file for contributors who wish to run the tests in isolated
virtual envs (with different python and dependencies versions) and empty
test directories locally (contributors who don't install or run tox
wouldn't be affected). Here also I don't think it is strictly necessary
nor have a strong opinion about it.
|
Sounds fair! Thank you for these insights. I guess we can safely merge this. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
part of changes discussed in #738